Update default settings #338
Conversation
JelteF
force-pushed
the
default-settings
branch
from
290d595
to
ce8c2f4
Compare
|
@Y-- can you look into these test failures related to |
|
One other probably good change is to never set So that's not really a real solution, just a related thing that we probably want to do anyway. |
test/regression/regression.conf
Outdated
| @@ -3,3 +3,4 @@ | |||
| shared_preload_libraries = 'pg_duckdb' | |||
| duckdb.force_execution = true | |||
| log_temp_files = -1 | |||
| duckdb.disabled_filesystems = '' | |||
There was a problem hiding this comment.
This should not be necessary. If we need to do this in tests it means this is a bad default (at the moment). Having a default that breaks half of pg_duckdb half of the functionality completely, even for the superuser
I think there's two things we need to do:
- temporarily change
duckdb_disabled_filesystemsto''when we know we want to interact with the filesystem. Like installing duckdb extensions, or when caching a parquet file. - Make sure that for postgres superusers this setting is always set to
''. They can already access files on the filesystem in various ways, so there's no point in blocking them when executing queries in DuckDB. Also they could just change theduckdb.disabled_filesystemssetting.
Those two should probably build on top of #342
JelteF
force-pushed
the
default-settings
branch
from
1b1a98a
to
0f44d39
Compare
JelteF
force-pushed
the
default-settings
branch
from
e4f1922
to
f917ef4
Compare
| values[Anum_duckdb_extension_enable - 1] = 1; | ||
|
|
||
| /* create heap tuple and insert into catalog table */ | ||
| Relation duckdb_extensions_relation = relation_open(ExtensionsTableRelationId(), RowExclusiveLock); |
There was a problem hiding this comment.
Is ExtensionsTableRelationId still used somewhere now?
There was a problem hiding this comment.
In ReadDuckdbExtensions
JelteF
force-pushed
the
default-settings
branch
2 times, most recently
from
f05ed5f
to
a5ea466
Compare
|
|
||
| /* inserting extension record */ | ||
| HeapTuple new_tuple = heap_form_tuple(tuple_descr, values, nulls); | ||
| CatalogTupleInsert(duckdb_extensions_relation, new_tuple); |
There was a problem hiding this comment.
This low level API was not causing the trigger to be fired. So this PR actually also fixes a bug where the duckdb.install_extension() call would not update the sequence, and thus not existing cause connections to load the extension.
|
Okay, I made the necessary changes to make these new defaults usable. It now depends on #342 though. |
JelteF
dismissed stale reviews from Y-- and wuputah
The base branch was changed.
Co-authored-by: Y. <[email protected]>
JelteF
force-pushed
the
default-settings
branch
from
622b54c
to
666ed03
Compare
Fixes #217
This changes the defaults for
duckdb.max_memoryandduckdb.disabled_filesystemsto the agreed upon settings. While doing that I found two issues that this also fixes:duckdb.disabled_filesystems = 'LocalFilesystem'was basically a way to make DuckDB execution unusable, due to us doing file system access when initializing the database/connection. So now we enable the setting later. Also we now don't callINSTALL {extension}during connection initialization anymore.duckdb.install_extension()was called. Only for direct inserts to theduckdb.extensionstable. That's fixed by not using the low-levelCatalogTupleInsert, and instead switching to SPI.duckdb.disabled_filesystems, they should be able to do whatever they want. So now this setting is not actually configured for superusers anymore.